Integrate 1D toon by chengcli · Pull Request #95 · chengcli/pyharp

chengcli · 2026-03-02T19:09:02Z

No description provided.

Copilot

Pull request overview

Integrates a 1D Toon (McKay 1989) “longwave/Planck” mode into the existing Toon RT solver, and wires solver flags through YAML/Python to select SW vs LW behavior.

Changes:

Added a planck option (and attempted flags plumbing) for ToonMcKay89, plus YAML support for solver flags.
Updated ToonMcKay89 runtime selection to use the planck option for SW vs LW, with a temf requirement for LW.
Updated Toon tests and added new grey-opacity example scripts/YAML (and removed the large sub-neptune RT example).

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
tests/test_toon.py	Sets Toon `planck=True` for LW test path.
tests/test_toon.cpp	Toggles Toon SW/LW mode in C++ tests.
src/rtsolver/toon_mckay89.hpp	Adds `planck` option and a new `flags(...)` helper (currently problematic).
src/rtsolver/toon_mckay89.cpp	Switches SW/LW selection to `planck` option and enforces `temf` for LW.
src/radiation/radiation_band.cpp	Reads `flags` from band YAML into Toon options (and logs flags).
python/csrc/pyrtsolver.cpp	Exposes new Toon options to Python, including `planck` and attempted `flags` binding.
examples/run_sub_neptune_rt.py	Removed prior end-to-end sub-neptune RT driver example.
examples/example_grey_opacity.py	New minimal JIT opacity compilation/usage example (currently inconsistent shapes).
examples/example_grey_flux.py	New grey RT demo script (currently missing imports/undefined variable).
examples/example_grey.yaml	New example config enabling Toon LW via `flags: planck`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-02T19:14:14Z

+    flags_str = "";
+    if (zenith_correction()) flags_str += "zenith_correction,";
+    if (planck()) flags_str += "planck,";
+    flags_str.pop_back();  // remove trailing comma


flags_str.pop_back() will throw/UB if no flags are enabled (e.g., when flags_str is empty or none of the known flags are present). Guard against an empty string before popping, or build the string without requiring a trailing-comma removal.

Suggested change

flags_str.pop_back(); // remove trailing comma

if (!flags_str.empty()) {

flags_str.pop_back(); // remove trailing comma

}

Copilot · 2026-03-02T19:14:14Z

  auto flx = torch::zeros({nwave, ncol, nlyr + 1, 2}, prop.options());

-  if (!temf.has_value()) {  // shortwave
+  if (!options.planck()) {  // shortwave


options is a std::shared_ptr<ToonMcKay89OptionsImpl>, so options.planck() will not compile. This should use options->planck() (consistent with the later options->wave_lower() / options->wave_upper() accesses).

Suggested change

if (!options.planck()) { // shortwave

if (!options->planck()) { // shortwave

Copilot · 2026-03-02T19:14:15Z

      .ADD_OPTION(std::vector<double>, harp::ToonMcKay89OptionsImpl, wave_lower)
      .ADD_OPTION(std::vector<double>, harp::ToonMcKay89OptionsImpl, wave_upper)
-      .ADD_OPTION(bool, harp::ToonMcKay89OptionsImpl, zenith_correction);
+      .ADD_OPTION(std::string, harp::ToonMcKay89OptionsImpl, flags)


.ADD_OPTION(std::string, ..., flags) expects an accessor pair with signatures const std::string& flags() const and ToonMcKay89OptionsImpl& flags(std::string const&). The current flags(std::string flags_str="") const in ToonMcKay89OptionsImpl does not match those signatures (returns by value, takes by value, and is const while mutating), so this binding will fail to compile. Either adjust flags to match the ADD_OPTION convention or bind it with a custom .def(...) instead of ADD_OPTION.

Suggested change

.ADD_OPTION(std::string, harp::ToonMcKay89OptionsImpl, flags)

.def(

"flags",

[](harp::ToonMcKay89OptionsImpl &self, std::string flags_str) {

return self.flags(std::move(flags_str));

},

py::arg("flags_str") = std::string{})

Copilot · 2026-03-02T19:14:15Z


+  toon.options.planck(false);
  for (auto [w0, g] : {std::make_pair(0.1, 0.5), std::make_pair(0.5, 0.5),
                       std::make_pair(0.9, 0.5)}) {


toon is a TORCH_MODULE holder, so member access to the underlying impl should go through ->. toon.options.planck(false); won’t compile; use toon->options->planck(false) (or toggle via the original op shared_ptr).

Copilot · 2026-03-02T19:14:15Z


+  toon.options.planck(true);
  for (auto [w0, g] : {std::make_pair(0.1, 0.5), std::make_pair(0.5, 0.5),


Same issue as earlier: toon.options.planck(true); won’t compile on a TORCH_MODULE holder. Use toon->options->planck(true) (or update op and reconstruct toon).

Copilot · 2026-03-02T19:14:15Z

+
+    opacity_output = opacity_model(conc, pres, temp)
+    print(opacity_output)


opacity_model is not defined before it’s called, so this example will crash. It looks like this should call one of the compiled/constructed opacity models (e.g., grey_sw / grey_lw) or use the Radiation pipeline instead.

Copilot · 2026-03-02T19:14:16Z

+  std::string flags(std::string flags_str = "") const {
+    // turn on options based on flags_str
+    zenith_correction(false);
+    if (flags_str.find("zenith_correction") != std::string::npos) {
+      zenith_correction(true);
+    }
+
+    planck(false);
+    if (flags_str.find("planck") != std::string::npos) {
+      planck(true);
+    }


flags(...) is declared const but calls non-const setters like zenith_correction(false) / planck(false), which won’t compile with the current ADD_ARG accessors. Consider making this a non-const setter (e.g., ToonMcKay89OptionsImpl& flags(std::string const&)) and a separate getter for the current flags (or store the original flags string).

Copilot · 2026-03-02T19:14:16Z

+      op->toon()->flags(trim_copy(band["flags"].as<std::string>()));
+    }
+    if (op->verbose()) {
+      std::cout << "  Solver flags: " << op->disort()->flags() << std::endl;


In the solver_name == "toon" branch, the verbose log prints op->disort()->flags(). That’s the wrong solver options object here and will be null/uninitialized for Toon; this should print the Toon solver’s flags/options instead.

Suggested change

std::cout << " Solver flags: " << op->disort()->flags() << std::endl;

std::cout << " Solver flags: " << op->toon()->flags() << std::endl;

Copilot · 2026-03-02T19:14:16Z

+@dataclass
+class RadiativeTransferConfig:
+    update_dt: float
+    sw_surface_albedo: float
+    lw_surface_albedo: float
+    stellar_flux_nadir: float
+
+@dataclass
+class RadiativeTransferState:
+    rad: Radiation
+    cfg: RadiativeTransferConfig
+    dz: torch.Tensor  # (nlyr,)
+    il: int
+    iu: int
+    last_heating: torch.Tensor  # (ny, nx, nlyr), W/m^3 == Pa/s
+    next_update_time: float
+    sw_band_weight_sum: float
+
+def run_rt(rad: Radiation, 
+           conc: torch.Tensor, 
+           dz: torch.Tensor,
+           atm: dict[str, torch.Tensor],
+           config: dict[str, Any]) -> Tuple[torch.Tensor, torch.Tensor, torch.Tensor]:
+    ncol = conc.shape[0]


@dataclass and Tuple[...] are used but neither dataclass nor Tuple is imported (and there’s no from __future__ import annotations), so this example will raise at import/definition time. Add the missing imports (or postpone annotation evaluation) to keep the example runnable.

Wire Toon into the YAML-driven radiation-band path alongside DISORT by parsing an optional toon: block for solver-specific options and registering the Toon solver module during band reset. Fix the Toon longwave backend for pure-absorption cases by padding missing scattering channels before dispatch so examples like jupiter_1d_rt can run without reading past the optical-property tensor. Add coverage for Toon YAML option loading and solver registration, and include a Toon variant of the Jupiter 1D RT example config for backend comparison.

Replace the Toon boolean option fields with a comma-separated flags string so the backend configuration matches the DISORT interface across YAML, C++, and Python bindings. Add a planck flag and use it as the explicit gate for thermal emission in Toon instead of inferring longwave mode from temf being present. This keeps temf from changing solver mode unless the configuration requests Planck emission. Update YAML parsing to accept top-level and nested Toon flags, preserve compatibility by translating legacy nested boolean keys into flags, and refresh the Toon example and regression tests to validate the new interface and the planck gating behavior.

Copilot AI review requested due to automatic review settings March 2, 2026 19:09

chengcli requested a review from luminoctum as a code owner March 2, 2026 19:09

Copilot started reviewing on behalf of chengcli March 2, 2026 19:09 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

chengcli added 2 commits April 23, 2026 12:52

chengcli force-pushed the cli/integrate_toon branch from 2d80a50 to 2d6e392 Compare April 23, 2026 18:01

chengcli merged commit 9d3a032 into main Apr 23, 2026
3 checks passed

chengcli deleted the cli/integrate_toon branch April 23, 2026 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate 1D toon#95

Integrate 1D toon#95
chengcli merged 2 commits into
mainfrom
cli/integrate_toon

chengcli commented Mar 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	if (!options.planck()) { // shortwave
	if (!options->planck()) { // shortwave

-      .ADD_OPTION(std::string, harp::ToonMcKay89OptionsImpl, flags)
+      .def(
+          "flags",
+          [](harp::ToonMcKay89OptionsImpl &self, std::string flags_str) {
+            return self.flags(std::move(flags_str));
+          },
+          py::arg("flags_str") = std::string{})


		toon.options.planck(true);
		for (auto [w0, g] : {std::make_pair(0.1, 0.5), std::make_pair(0.5, 0.5),


		opacity_output = opacity_model(conc, pres, temp)
		print(opacity_output)

	std::cout << " Solver flags: " << op->disort()->flags() << std::endl;
	std::cout << " Solver flags: " << op->toon()->flags() << std::endl;

Conversation

chengcli commented Mar 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants